Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inherit overridden attribute properties #204

Merged
merged 5 commits into from
Sep 27, 2023

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Sep 17, 2023

Fixes #136

In some cases, we want to have the following schema:

  • server.address is a general-purpose attribute defined in server conv
  - id: server
    prefix: server
    attributes:
      - id: address
        brief: 'Domain name.'
       ....
  • http semconv wants to define it more narrowly. E.g. update a brief or add HTTP-specific note
- ref: server.address
  brief: Server component of Host header.
  • Then HTTP version of this attribute can be used in HTTP traces and metrics.

(see fill example in tests)

Today it works only if semconv extends base HTTP one without mentioning the attribute like

  - id: http.client.request.duration.metric
    type: metric
    metric_name: http.client.request.duration
   ...
    extends: http.client.attributes

Then an attribute is populated from parent semconv as is.

However, if child semconv specifies an attribute even further, it fills it in from the original definition, without considering parent hierarchy first.

I.e.

  - id: http.client.spans
    type: span
   ...
    extends: http.client.attributes
    attributes:
      - ref: server.address
        sampling_relevant: true

resolves to

Attribute Type Description Examples Requirement Level
server.address string Domain name. foo Recommended

instead of

Attribute Type Description Examples Requirement Level
server.address string Server component of Host header. foo Required

The workaround we apply in semconvs is to copy all the notes which is error-prone.

The change implemented here walks up the extended hierarchy and fills in inherited properties in the attribute first.

@lmolkova lmolkova requested review from a team September 17, 2023 19:55
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically, instead of starting from the primary definition, we start from the most-derived defintion, if we inherit it already? Sounds good 👍

Could you add this to the CHANGELOG, please?

@lmolkova
Copy link
Contributor Author

So basically, instead of starting from the primary definition, we start from the most-derived defintion, if we inherit it already? Sounds good 👍

yep!

Could you add this to the CHANGELOG, please?

done

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Referenced attributes are partially generated in markdown
6 participants